fix(desktop): keep publishing a mention when a channel agent fails to start - #5159
fix(desktop): keep publishing a mention when a channel agent fails to start#5159TheSeydiCharyyev wants to merge 1 commit into
Conversation
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing on Wes's behalf.
The membership-based classification is a real fix for #5099: an already-in-channel agent does not need this desktop to launch it for the relay mention to be valid, while attach/preparation failures for a non-member still block. The change is narrow, does not alter relay authorization or membership, and the new unit/E2E coverage exercises the core regression.
I am requesting one correction before putting this into this morning's release:
readiness.warning says “Message sent without starting…”, but it is displayed before Huddle synchronization, media upload, and onSend. Each of those can still fail or abort. In particular, a start failure followed by sync_agents_to_active_huddle failure produces a “Message sent” toast and then returns without sending anything; upload/send failure can likewise restore the composer after the success-tense toast has already been shown. That is false delivery confirmation on an error path.
Please either:
- make the pre-send warning future/present tense (for example, “Could not start …; sending the mention anyway”) and use
toast.warning, or - emit the current past-tense warning only after
finishSendhas successfully awaitedsend.
Add focused coverage for the selected ordering/wording so a later pre-send failure cannot claim delivery. Once corrected, I see no safety blocker to release.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
I traced the membership/preparation state and reproduced the regression coverage at exact head 8857e525cae1045ab79135d5cd2cf886a835b842. No blocking findings.
This is a real fix for #5099: a local launch failure previously aborted publication even when the mentioned identity was already a member and could receive/respond through a remote runtime. The new boundary is sound: only a pubkey present in the pre-existing, normalized channel-member set gets non-blocking treatment. Failed attaches and agents only prepared for a newly created/expanded channel remain blocking, including the existing expanded-DM failure behavior.
Local verification at this exact head:
- full Desktop unit suite: 4,485 passed, 0 failed
- Desktop TypeScript build/typecheck: passed
- focused E2E (
mentioning an in-channel agent still sends when its start fails): passed - Biome check on all four changed files: passed
- clean merge-tree with current
origin/main(e47894a133c2a685efd7aca5dd210daad8cd13b8)
The fork workflow only exposed DCO/Semgrep/zizmor, so trusted repository CI still has not run. For a same-morning release I recommend running the normal Desktop CI before merge, but the code itself is narrow and release-safe. Minor non-blocking UX nit: the successful-send notice is emitted with toast.error; toast.warning would better match the behavior and copy.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Correction after cross-review: Carl identified a sequencing defect I missed in my initial assessment, and I agree it should be fixed before release. The warning currently says “Message sent…” and is shown before Huddle sync, upload, and onSend; any later failure can therefore tell the user a message was sent when it was not. Please use pre-send/future-tense copy with toast.warning, or emit the past-tense notice only after successful publication, with coverage.
The underlying #5099 fix and membership boundary remain sound. Local verification at exact head 8857e525cae1045ab79135d5cd2cf886a835b842 passed: 4,485 Desktop unit tests, TypeScript build/typecheck, focused regression E2E, and Biome on the four changed files; the branch also merge-trees cleanly with origin/main at e47894a133c2a685efd7aca5dd210daad8cd13b8. Trusted repository CI still has not run because this is a fork.
Updated disposition: do not include this exact head in the release until the false-success notification is corrected.
… start
ensureManagedAgentMentionsReady collects every agent-preparation failure
into one list, and any entry in it returns before the send, so the mention
is gone. That is right when the agent is not in the channel yet, because
nothing else would put it there. It is wrong when the agent is already a
member: an agent that runs outside this desktop has no private key on this
machine and never will, so its launch fails every time while it sits in the
channel and would answer.
A preparation failure now blocks only when the agent is not already a member
of the target channel. For a member the local start is an optimization, so
the message is published and the failure is reported instead.
That notice is emitted before Huddle sync, media upload and the send itself,
any of which can still abort, so it is worded pre-send ("sending the mention
anyway") and raised with toast.warning. Past-tense copy there would be a
false delivery confirmation on an error path.
Blocking behaviour is unchanged for a failed attach and for an agent only
prepared for a channel this send creates or expands, which is what
channels.spec.ts pins for expanded DMs.
Closes block#5099.
Signed-off-by: Seydi Charyyev <seydi.charyev@gmail.com>
8857e52 to
acde6dc
Compare
|
Fixed in The notice is now pre-send tense — Coverage for the ordering, as requested. New e2e The branch is also rebuilt on current Local verification at |
2 participants
Summary
ensureManagedAgentMentionsReadycollects every agent-preparation failure into one list, and any entry in that list returns before the send. The mention is not delayed or queued — it is gone.That is fine when the agent is not in the channel yet, because nothing else would put it there. It is wrong when the agent is already a member. A managed agent that runs outside this desktop — a container on the user's own server, the shape
docs/remote-agents.mddescribes — has no private key on this machine and never will, so its start fails every time. It sits in the channel and would answer, and the message is dropped anyway.This PR makes a preparation failure block the send only when the agent is not already a member of the channel the message is going to. For an agent that is already there, the local start is an optimization: the relay routes the mention whether or not this desktop runs a copy, so the message is published and the failure is reported as a warning instead.
Everything else keeps today's behaviour:
channels.spec.tspins in "drops an expanded DM after agent startup fails", and that test is untouched and still passes.The classification and both message strings live in
useMentionSendFlow.helpers.tsso the hook itself does not grow:useMentionSendFlow.tsstays at exactly its current line count.Related issue
Closes #5099. Split out of #4839 by the reporter; that PR fixes mention visibility and deliberately does not touch the start path. Searched open PRs for the issue number and for these files: none found on this path (#4258 touches
useMentionSendFlow.tsfor link previews).Testing
New e2e,
mentions.spec.ts→ "mentioning an in-channel agent still sends when its start fails": an in-channel provider-backed agent whose start is rejected. On currentmainthe mention chip never appears — the message is dropped. With this change it publishes and the failure shows as a warning.pnpm --dir desktop test— 4394 passed (5 new unit tests for the classification)pnpm --dir desktop typecheckpnpm --dir desktop check— biome, file-size ratchet, px-text, pubkey-truncationOne note on the suite: full
channels.spec.ts+mentions.spec.tsruns are flaky on this machine — a toast can cover the send button and time the click out. I ran the same two files on unmodifiedmainas a control: 5 failures there, 1–2 with this change, a different set each time, and every one of them passes in isolation. The failures are not from this change.